Skip to content

Add reboot button to Shelly devices#60417

Merged
frenck merged 4 commits intohome-assistant:devfrom
mib1185:shelly/reboot-button
Nov 29, 2021
Merged

Add reboot button to Shelly devices#60417
frenck merged 4 commits intohome-assistant:devfrom
mib1185:shelly/reboot-button

Conversation

@mib1185
Copy link
Copy Markdown
Member

@mib1185 mib1185 commented Nov 26, 2021

Proposed change

This reworks the recent implemented buttons to use entity descriptors and adds the new button to trigger a reboot of the Shelly device.
Since this feature is fairly not common to be used, the button is disabled by default.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link
Copy Markdown

Hey there @balloob, @bieniu, @thecode, @chemelli74, mind taking a look at this pull request as it has been labeled with an integration (shelly) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@mib1185 mib1185 marked this pull request as ready for review November 26, 2021 21:40
Copy link
Copy Markdown
Member

@bieniu bieniu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@frenck
Copy link
Copy Markdown
Member

frenck commented Nov 28, 2021

Since this feature is fairly not common to be used, the button is disabled by default.

It's marked as the config entity category and doesn't update frequently, so there should not be a reason to disable it from that perspective? Is there any specific other reason?

@thecode
Copy link
Copy Markdown
Member

thecode commented Nov 28, 2021

I agree with @frenck, we try to minimize irrelevant entities (for example BETA OTA is disabled), but since it is a configuration entity would be nice to leave it enabled by default.

@mib1185
Copy link
Copy Markdown
Member Author

mib1185 commented Nov 28, 2021

I agree with @frenck, we try to minimize irrelevant entities (for example BETA OTA is disabled), but since it is a configuration entity would be nice to leave it enabled by default.

The beta OTA button is also a configuration entity.

[...}doesn't update frequently, so there should not be a reason to disable it from that perspective?

I agree, that this will not pollute the state machine, but ...

Is there any specific other reason?

... since both (beta OTA and reboot button) are assumed to not be common used - only by advanced users.
So to not "overwhelm" the common users (and maybe avoid accidental button press) these buttons were disabled by default, but can be enabled by the user if really needed.

@frenck
Copy link
Copy Markdown
Member

frenck commented Nov 28, 2021

We used to disable less common entities in the past, to prevent generated dashboards and such to be cleaner (and thus clean from less commonly used entities). The introduction of the entity categories made that a lesser problem.

The beta case, I agree, that should be disabled by default, but this case: I don't see a reason why it should be disabled.

@mib1185
Copy link
Copy Markdown
Member Author

mib1185 commented Nov 28, 2021

IMO a regular OTA update is somehow normal during usual lifetime of a device, but a beta firmware or rebooting the device would fairly not commonly be used (for me there was no use case till now, to reboot a Shelly device), so i assumed to disable them (for same reason) should be ok.

For sure, if we want the reboot button be enabled by default, than i will change it 👍

@frenck
Copy link
Copy Markdown
Member

frenck commented Nov 29, 2021

For sure, if we want the reboot button be enabled by default, than i will change

Yes :)

@mib1185
Copy link
Copy Markdown
Member Author

mib1185 commented Nov 29, 2021

For sure, if we want the reboot button be enabled by default, than i will change

Yes :)

Your wish is my command 😃

Copy link
Copy Markdown
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @mib1185 👍

@frenck frenck merged commit 83acfda into home-assistant:dev Nov 29, 2021
@mib1185 mib1185 deleted the shelly/reboot-button branch November 29, 2021 19:04
@github-actions github-actions Bot locked and limited conversation to collaborators Nov 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants